-
Notifications
You must be signed in to change notification settings - Fork 2
Refactor(client): 라우팅 구조 개선 및 접근 제어 로직 리팩토링 #211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
🎨 Storybook 배포 완료PR 작성자: @jyeon03 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
크으 ~ 리팩토링의 첫 PR 영광스럽네요
코멘트 몇 개 남겼는데 확인 부탁드릴게요 !! 라우팅 관련해서 함께 이야기해보고 더 좋은 구조 함께 고민해보면 좋을 것 같아요 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 publicRoutes, privateRoutes로 라우트를 분리하는 방식이 유지보수성과 가독성 측면에서 더 낫다고 생각해요 !!
인증 필요/불필요라는 기준이 명확한 도메인 규칙이기 때문에 오히려 지금처럼 라우트를 한 배열에 섞어두면 규모가 커질수록 분류 비용과 실수 가능성이 크다고 생각합니다 !
특히 새로운 페이지 추가 시에 이 페이지가 인증이 필요한가?를 판단하고 그 결과에 따라 private/public 중 한 곳에만 넣으면 되기 때문에 변경 범위가 작고 리뷰 포인트도 명확해진다고 생각하는데 지연님의 생각은 어떨까요 ?
또한 SSoT 위배 라고 말씀해주셨는데 분리 자체는 중복 정의가 아니라 책임 분리에 가깝다고 생각해요 !
SSoT가 실제로 깨지는 케이스는 이 페이지가 private인지 여부와 같은 동일한 규칙이 routes 파일/guard/메뉴 매핑 등 여러 곳에서 중복으로 관리되어 불일치가 생길 때인데 현재 구조는 접근 정책을 privateRoutes/publicRoutes라는 한 기준으로 모아두는 형태라 오히려 정책을 추적하기 쉽다고 생각합니다 !!
| Component: PublicLayout, | ||
| children: publicRoutes, | ||
| Component: GuardedPublicLayout, | ||
| children: routes.filter((r) => !r.auth), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런식으로 filter 함수를 사용하면 가독성이 떨어질 것 같아요 ! routes 파일을 다시 분리하게 된다면 해결될 문제 같아요 !
| const { pathname } = useLocation(); | ||
|
|
||
| const publicOnlyPaths: string[] = [ | ||
| PATH.LANDING, | ||
| PATH.LOGIN, | ||
| PATH.LOGIN_CALLBACK, | ||
| ]; | ||
|
|
||
| if (accessToken && publicOnlyPaths.includes(pathname)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const GuardedPrivateLayout = () => (
<PrivateRouteGuard>
<PrivateLayout />
</PrivateRouteGuard>
);PublicRouteGuard를 PublicLayout 전체에 감싸면
PublicLayout 아래에 있는 모든 라우트가 public 영역이 되고 있기 때문에
현재 pathname이 publicOnlyPaths에 해당하나를 검사할 이유가 없을 것 같아요 !!
jm8468
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상세하게 pr을 써주셔서 더 편하게 리뷰할 수 있었던 것 같아요👍👍👍
수고 많으셨어요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 publicRoutes, privateRoutes로 라우트를 분리하는 방식이 유지보수성과 가독성 측면에서 더 낫다고 생각해요 !!
인증 필요/불필요라는 기준이 명확한 도메인 규칙이기 때문에 오히려 지금처럼 라우트를 한 배열에 섞어두면 규모가 커질수록 분류 비용과 실수 가능성이 크다고 생각합니다 !
특히 새로운 페이지 추가 시에 이 페이지가 인증이 필요한가?를 판단하고 그 결과에 따라 private/public 중 한 곳에만 넣으면 되기 때문에 변경 범위가 작고 리뷰 포인트도 명확해진다고 생각하는데 지연님의 생각은 어떨까요 ?
메타데이터에 대해 잘 몰라서 메타데이터 사용과 관련해서 찾아봤습니다😊
인증 여부, 비로그인 전용, 유료 구독 여부 등으로 다양하게 메타 데이터를 입력하고, 이러한 메타데이터들을 사용해 유지보수성도 높이고, 페이지 별로 어떤 권한을 가지는 지 알 수 있어서 메타 데이터 이용에 확실한 장점이 있다고 생각해요
반대로, 저희는 현재 인증 여부(auth) 만으로 페이지를 나누고 있어서, 현 상황에서는 오히려 더 명확하게 구분이 되도록 private-route.ts, public-route.ts로 나누는 것도 장점이 있다고 생각해요
저도 다른 분들 생각이 궁금한데, 프로젝트 규모가 커질 수록 페이지 별로 역할이 어느정도 다양해질 거고, 페이지를 인증 여부 별로만 분류하지는 않을 거라고 생각합니다. 그래서 결국에 메타데이터를 사용하게 될 것 같은데, 미래를 보고 우선적으로 해야할 지, 현재 상황에 맞게 가야할 지 궁금해요!!😊😊
메타데이터 사용 시 실수를 줄이기 위해서는 작성하신 것처럼 타입을 작성해주고, 자주 사용되는 메타데이터를 default로 상정하고 코드를 작성해주는 등의 방법을 취해주면 좋을 것 같습니다!
| { | ||
| path: PATH.NEW_MEMO, | ||
| Component: NewMemoPage, | ||
| auth: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auth-guard 방식을 auth: true일 때가 아닌 auth: false일 때를 기준으로 하는 건 어떻게 생각하시나요??
Private Routes에 있는 auth: true를 아예 없애고, Public Routes에 auth: false를 작성하는 방식입니다!
아무래도 auth: true일 때 사용되는 페이지가 많으니, 적게 사용되는 비인증 전용 페이지에만 메타데이터를 작성하는 것이 추후 생길 휴먼 에러를 더 줄일 수 있다고 생각해요😊😊👍
// Public Routes
{
path: PATH.LOGIN,
Component: LoginPage,
meta: { auth: false },
},
{
path: PATH.LOGIN_CALLBACK,
Component: LoginCallbackPage,
meta: { auth: false },
},
{
path: PATH.LANDING,
Component: LandingPage,
meta: { auth: false },
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PrivateRouteGuard, PublicRotueGuard로 나눠주셨는데, 만약 페이지가 더 늘어나고, 페이지가 인증 여부 외에도 다른 요소에 따라 보이는게 달라져야 한다면, 하나로 합치는 것이 좋을 것 같은데 어떻게 생각하시나여??
그리고 AuthGuard 내에서 토큰 유무, 메타데이터 유무를 조건으로 제어문을 작성한다면 한 곳에서 작성하므로, 분산이 적어지고, 유지보수 비용 또한 낮아질 수 있다고 생각해요
단일책임원칙에 대해서도 얘기해주셨는데, AuthGurad를 'routes/index.ts에 정의된 내용을 실행하는 역할'로 본다면 하나로 관리하는 것도 단일책임원칙에 부합하지 않을까라고 생각하고 있습니당.
책임을 더 세세하게 보고, 각각 분산한다면 오히려 응집도가 떨어지는게 아닐까라고 생각하고 있는데 다른 분들 의견도 궁금하네용
이 코멘트도 확장성을 보고 얘기드리는 거여서 오버 엔지니어링 여부를 봐야할 것 같습니다!😊😊
📌 Summary
분산되어 있던 라우팅 구조를 중앙에서 관리하도록 단일화하고, 불완전했던 접근 제어 로직을 개선하여 유지보수성과 확장성 을 향상시키는 리팩토링 작업을 진행했습니다!!!!!
📚 Tasks
🔍 Describe
왜 이 리팩토링이 필요했나??미인증 사용자 차단과인증된 사용자 처리라는 여러 책임을 가지려 했으나, 후자를 guard에서 제대로 처리하지 못해 로직에 허점이 존재했습니다.어떻게 재설계했나??모든 경로 설정을 routes/index.ts 단일 파일로 통합하여 SSoT을 확보했습니다. 각 경로에 auth: true 메타데이터를 추가하여 인증 필요 여부를 선언적으로 명시하도록 변경했습니다.
변경 후
단일 책임 원칙에 따라, 하나의 복잡한 AuthGuard를 두 개의 명확한 책임을 가진 PrivateRouteGuard와 PublicRouteGuard로 분리했습니다.
변경 전
메인 라우터가 중앙화된 routes 배열을 동적으로 필터링하여 Public/Private 경로 그룹을 자동으로 구성하도록 수정했습니다.
변경 전
👀 To Reviewer
state={{ from: location }}패턴을 사용하면 로그인 후 원래 접근하려던 경로로 복귀시키는 UX를 구현해보면 어떨까싶어요! 추후 딥링크 대응이나 기획 변경 가능성을 고려하면 Guard에서 이 정보를 함께 전달하는 구조도 한 가지 방법일 것 같다는 생각이 들었습니당 🥸